Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Nov 17, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Nov 20, 2025 8:52pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 20, 2025 8:52pm
rivet-inspector Ignored Ignored Preview Nov 20, 2025 8:52pm
rivet-site Ignored Ignored Preview Nov 20, 2025 8:52pm

Copy link
Contributor Author

MasterPtato commented Nov 17, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Nov 17, 2025

Code Review

Summary

This PR performs a straightforward renaming of the retry field to hibernate in the runner protocol's ToServerWebSocketClose message. The change improves semantic clarity by better describing the field's purpose: indicating whether a WebSocket connection should enter a hibernation state rather than fully close.

Positive Aspects ✅

  1. Semantic Clarity: The rename from retry to hibernate is a good improvement. The new name more accurately describes the behavior - the connection is entering a hibernation state that can be resumed, rather than a simple retry.

  2. Comprehensive Coverage: The change is applied consistently across:

    • Protocol schema (v3.bare)
    • Rust protocol implementation
    • TypeScript protocol bindings
    • All consuming code in gateway and runner packages
    • Protocol version conversion functions (v2 ↔ v3)
  3. Backward Compatibility: The protocol version conversion functions correctly maintain backward compatibility by mapping close.retry (v2) ↔ close.hibernate (v3) in both directions at versioned.rs:1288 and versioned.rs:1337.

  4. Logical Improvements: The code reorganization in pegboard-gateway/src/lib.rs:404-409 improves readability by moving the "Successful closure" comment to the else branch where it actually applies.

Issues Found 🔍

1. Unrelated Import Reordering (Minor)

File: engine/packages/api-peer/src/actors/kv_get.rs:1-3

The import order change appears unrelated to the PR's purpose:

-use base64::prelude::BASE64_STANDARD;
 use base64::Engine;
+use base64::prelude::BASE64_STANDARD;

Recommendation: This formatting change should either be:

  • Reverted (preferred, to keep the PR focused)
  • Or explained if it's intentional to match the project's import ordering standards

2. Unrelated Formatting Changes (Minor)

File: engine/packages/api-public/src/actors/kv_get.rs:68-72

The multiline formatting of the format! call appears unrelated to the rename:

-"/actors/{}/kv/keys/{}", path.actor_id, urlencoding::encode(&path.key)
+&format!(
+    "/actors/{}/kv/keys/{}",
+    path.actor_id,
+    urlencoding::encode(&path.key)
+)

Recommendation: Consider reverting this change to keep the PR focused on the retry → hibernate rename.

3. Assert Function and Whitespace Changes (Minor)

File: engine/sdks/typescript/runner-protocol/src/index.ts

Multiple unrelated changes:

  • Moving import assert from "node:assert" to the top (line 1)
  • Removing the custom assert function at the bottom (lines 1930-1933)
  • Whitespace changes in callbacks (() => {}() => { })
  • Indentation changes (lines 611, 621)

Recommendation: While these are generally good cleanups:

  • The assert import move is good (following best practices)
  • Removing the duplicate assert function is correct (using Node's built-in instead)
  • However, the whitespace changes in empty functions and indentation adjustments should ideally be in a separate formatting PR

Per CLAUDE.md: "Do not run ./scripts/cargo/fix.sh. Do not format the code yourself." These formatting changes may have been unintentional.

Recommendations 📝

  1. Consider reverting unrelated changes to keep the PR focused solely on the retry → hibernate rename. This makes code review easier and git history cleaner.

  2. Test Coverage: While the change is straightforward, consider verifying that:

    • WebSocket hibernation behavior still works correctly
    • Protocol version conversion maintains backward compatibility
    • No tests were broken by the rename
  3. Documentation: If there's any user-facing documentation that mentions the "retry" field, it should be updated to reference "hibernate" instead.

Security & Performance ✅

  • No security concerns identified
  • No performance impact (purely a naming change)
  • Backward compatibility properly maintained through version conversion

Overall Assessment

This is a clean refactoring PR that improves code clarity. The core change (rename) is well-executed and comprehensive. The only concerns are minor unrelated formatting changes that could be cleaned up to keep the PR focused.

Recommendation: Approve with minor cleanup suggestions.

@MasterPtato MasterPtato force-pushed the 11-13-fix_udb_update_backoff_algo branch from 9b619f4 to 267ddc0 Compare November 17, 2025 19:35
@MasterPtato MasterPtato force-pushed the 11-13-chore_rename_retry_-_hibernate_in_runner_protocol branch from 7ec4550 to bbe84e7 Compare November 17, 2025 19:35
@claude
Copy link

claude bot commented Nov 17, 2025

Code Review

Thank you for this PR! The rename from retry to hibernate better reflects the semantic meaning of the field. Here is my review:

✅ Strengths

  1. Clear intent: The rename from retry to hibernate is semantically more accurate for describing WebSocket connection hibernation behavior
  2. Comprehensive coverage: The change is applied consistently across:
    • Protocol schema (v3.bare)
    • Rust implementations (pegboard-gateway, pegboard-runner)
    • TypeScript SDK (runner-protocol, runner, websocket-tunnel-adapter)
    • Version conversion logic (v2 ↔ v3 conversions)
  3. Backward compatibility: The versioned.rs file correctly maintains the field mapping between v2 (retry) and v3 (hibernate) for protocol version compatibility

🐛 Issues Found

Critical: Missing update in stringify.ts

The file engine/sdks/typescript/runner/src/stringify.ts still references retry instead of hibernate:

// Line 61-64
const { code, reason, retry } = kind.val;
const codeStr = code === null ? "null" : code.toString();
const reasonStr = reason === null ? "null" : `"${reason}"`;
return `ToServerWebSocketClose{code: ${codeStr}, reason: ${reasonStr}, retry: ${retry}}`;

This should be updated to:

const { code, reason, hibernate } = kind.val;
// ...
return `ToServerWebSocketClose{code: ${codeStr}, reason: ${reasonStr}, hibernate: ${hibernate}}`;

Location: engine/sdks/typescript/runner/src/stringify.ts:61-64

📝 Minor Observations

  1. Unrelated formatting changes: The PR includes some minor formatting changes:

    • Import reordering in api-peer/src/actors/kv_get.rs
    • Formatting in api-public/src/actors/kv_get.rs
    • Whitespace changes in TypeScript files
    • Import statement consolidation in runner-protocol/src/index.ts

    While these are generally fine, it is worth noting they are technically outside the scope of the rename. Per the CLAUDE.md guidelines, cargo fmt should not be run automatically, so these changes appear to be manual adjustments.

  2. Comment improvement: The pegboard-gateway comment placement is better after the change - "Successful closure" is now in the else branch where it belongs (line pegboard-gateway/src/lib.rs:407).

✅ Security & Performance

  • No security concerns identified
  • No performance impact expected
  • The change is purely semantic and maintains the same behavior

🧪 Testing Recommendation

Given that this is a protocol-level change:

  1. Verify integration tests pass, especially those involving WebSocket hibernation
  2. Test protocol version compatibility (v2 ↔ v3 conversions)
  3. Ensure the stringify function works correctly after fixing the bug

Summary

This is a solid refactoring that improves code clarity. The main action item is to fix the missing rename in stringify.ts. Once that is addressed, this should be ready to merge!

@claude
Copy link

claude bot commented Nov 17, 2025

Code Review

This PR renames the retry field to hibernate in the runner protocol's ToServerWebSocketClose message. This is a valuable clarification that better reflects the actual semantics of the field - it controls whether a WebSocket should hibernate (as per HIBERNATING_WS.md) rather than retry.

Positives

  1. Better Naming: The rename from retry to hibernate accurately reflects the behavior documented in docs/engine/HIBERNATING_WS.md, where step 7 explicitly mentions "the runner will close the websocket by sending ToServerWebSocketClose with .hibernate = true"

  2. Comprehensive Coverage: The change properly updates:

    • Schema definition (v3.bare)
    • TypeScript bindings (runner-protocol/src/index.ts)
    • Rust implementations (pegboard-gateway, pegboard-runner)
    • Version conversion functions (v2 to v3)
    • WebSocket tunnel adapter
    • All usages in the tunnel implementation
  3. Correct Version Handling: The versioned.rs file correctly maps between v2's retry and v3's hibernate in both directions, maintaining backward compatibility

  4. Code Quality: The formatting changes (import ordering in kv_get.rs, spacing fixes) follow Rust conventions

Issues Found

  1. Missing Update in stringify.ts (Bug)

    The file engine/sdks/typescript/runner/src/stringify.ts:61-64 still references the old retry field. This should be updated to use hibernate instead. This will cause a TypeScript compilation error since the field was renamed in the type definition.

  2. Outdated Comment (Minor)

    engine/sdks/typescript/runner/src/tunnel.ts:80 has a comment that says "The WebSocket close event with retry is automatically sent when the". Consider updating "with retry" to "with hibernate" for consistency.

  3. Inconsistent Method Name (Semantic)

    The method __closeWithRetry() in websocket-tunnel-adapter.ts:207 should arguably be renamed to __closeWithHibernate() for consistency. Currently the method is called __closeWithRetry but it passes hibernate: true to closeInner and the protocol field is now hibernate. While this works functionally, the naming is now misleading.

  4. No Test Coverage (Gap)

    There don't appear to be tests verifying the hibernate field behavior. Consider adding tests to ensure:

    • Hibernatable websockets close with hibernate: true
    • Non-hibernatable websockets close with hibernate: false
    • The version conversion between v2 and v3 works correctly
  5. Generated TypeDoc Not Updated

    The file website/public/typedoc shows retry instead of hibernate. The TypeDoc likely needs to be regenerated.

Security

No security concerns identified. This is a straightforward rename that doesn't change the protocol's security properties.

Performance

No performance impact - this is a pure rename.

Recommendations

  1. Must Fix: Update stringify.ts to use hibernate instead of retry (lines 61, 64)
  2. Should Fix: Consider renaming __closeWithRetry() to __closeWithHibernate() for semantic consistency
  3. Nice to Have: Update the comment on line 80 of tunnel.ts
  4. Nice to Have: Add test coverage for hibernate behavior
  5. Nice to Have: Regenerate TypeDoc documentation

Summary

This is a good refactoring that improves code clarity. However, there's at least one file (stringify.ts) that was missed and will cause a compilation error. The PR should not be merged until that's fixed.

@claude
Copy link

claude bot commented Nov 18, 2025

PR Review: Rename retry -> hibernate in runner protocol

Summary

This PR renames the retry field to hibernate in the WebSocket close message of the runner protocol. This is a good semantic improvement as hibernate more accurately describes the intent of persisting WebSocket state for later resumption.

Code Quality - Strengths

  • Clean, focused rename across the protocol stack (schema, Rust, TypeScript)
  • Proper bidirectional protocol version conversion (v2-v3) maintained in versioned.rs
  • Consistent naming throughout all layers of the stack

Issues Found

1. Bug: Incomplete rename in stringify.ts (line 61, 64)
The stringify function still uses the old retry field name. It should use hibernate instead.
File: engine/sdks/typescript/runner/src/stringify.ts:61-64

2. Type inconsistency in websocket-tunnel-adapter.ts (line 20, 35)
The callback parameter still uses retry in its type signature instead of hibernate.
File: engine/sdks/typescript/runner/src/websocket-tunnel-adapter.ts:20,35

3. Outdated comment in tunnel.ts (line 80)
Comment still references retry instead of hibernate.
File: engine/sdks/typescript/runner/src/tunnel.ts:80

4. Unrelated formatting changes
The PR includes several unrelated formatting changes in api-peer, api-public, tunnel.ts, and runner-protocol index.ts. While these are good cleanups, they make the PR harder to review. Consider keeping refactoring PRs focused on a single change.

Security and Performance

  • No security concerns identified
  • No performance implications from this rename
  • Protocol versioning properly handles backward compatibility

Test Coverage

  • No tests found that explicitly cover the hibernate/retry field
  • Recommendation: Consider adding a test that validates WebSocket hibernation behavior

Action Items

Must Fix: Update stringify.ts and websocket-tunnel-adapter.ts to use hibernate
Should Fix: Update comment in tunnel.ts, separate formatting changes
Nice to Have: Add test coverage for WebSocket hibernation behavior

Verdict

Good refactoring that improves code clarity! Just needs completion of the rename in a couple of TypeScript files to maintain consistency.

@NathanFlurry NathanFlurry force-pushed the 11-13-chore_rename_retry_-_hibernate_in_runner_protocol branch from 274d094 to d44f1d1 Compare November 20, 2025 22:32
@MasterPtato MasterPtato force-pushed the 11-13-fix_udb_update_backoff_algo branch from 666b02e to 32b420c Compare November 20, 2025 22:38
@MasterPtato MasterPtato force-pushed the 11-13-chore_rename_retry_-_hibernate_in_runner_protocol branch from d44f1d1 to 274d094 Compare November 20, 2025 22:38
@claude
Copy link

claude bot commented Nov 20, 2025

PR Review: Rename retryhibernate in Runner Protocol

Overview

This PR renames the retry field to hibernate in the ToServerWebSocketClose message across the runner protocol (v3). This is a clean naming improvement that better reflects the semantic meaning of the field.

✅ Strengths

  1. Comprehensive Coverage: The rename is consistently applied across all layers:

    • Protocol schema (v3.bare)
    • Rust generated code (runner-protocol)
    • TypeScript generated code (runner-protocol SDK)
    • TypeScript runner implementation
    • Pegboard gateway
    • Pegboard runner
  2. Backward Compatibility: The version conversion code in versioned.rs correctly maintains backward compatibility:

    • convert_to_server_tunnel_message_kind_v2_to_v3: Maps v2's retry → v3's hibernate (line 1288)
    • convert_to_server_tunnel_message_kind_v3_to_v2: Maps v3's hibernate → v2's retry (line 1337)

    This ensures older clients using protocol v2 can still communicate with v3 servers and vice versa.

  3. Semantic Clarity: The rename from retry to hibernate is more descriptive and aligns better with the actual behavior (putting the WebSocket connection into hibernation rather than just retrying).

  4. Method Naming Consistency: Internal method names were also updated:

    • __closeWithRetry()__closeWithHibernate() in TypeScript WebSocket adapter

🔍 Observations

  1. Comment Repositioning (pegboard-gateway/src/lib.rs:404-408): The comment "Successful closure" was moved from before the hibernate check to the else branch. This is actually more accurate since hibernation is not a "successful closure" in the traditional sense, but rather a state transition.

  2. Formatting Changes (tunnel.ts): The PR includes some minor formatting changes:

    • Line 402-404: Arrow function formatting in resolve/reject callbacks
    • Line 610-611: Multi-line formatting for ternary expression

    While these are cosmetic, they appear to be accidental changes. However, per CLAUDE.md, formatting is handled by pre-commit hooks, so this should be fine.

🎯 Code Quality

  • ✅ No security concerns introduced
  • ✅ No performance implications
  • ✅ Type safety maintained across Rust and TypeScript
  • ✅ No breaking changes for protocol v2 clients due to proper version conversion
  • ✅ Follows existing naming conventions (camelCase in TypeScript, snake_case in Rust/schemas)

📝 Minor Suggestions

  1. Protocol Documentation: Consider adding a comment in v3.bare explaining what "hibernate" means in this context, especially since this is a protocol-level change:

    type ToServerWebSocketClose struct {
        code: optional<u16>
        reason: optional<str>
        # If true, indicates the connection should be hibernated rather than permanently closed
        hibernate: bool
    }
    
  2. Formatting Consistency: The accidental formatting changes in tunnel.ts (lines 402-404, 610-611) could be reverted if they weren't intentional, though they don't affect functionality.

✅ Test Coverage

The changes are primarily renaming operations that preserve existing behavior. The backward compatibility conversion ensures no regression for existing protocol v2 clients. However, consider:

  • Testing WebSocket hibernation flow end-to-end with both v2 and v3 clients
  • Verifying that hibernation works correctly after the rename

🎉 Conclusion

This is a well-executed refactoring PR. The rename improves code clarity, maintains backward compatibility through proper version conversion, and is applied consistently across the entire codebase. The changes are straightforward and low-risk.

Recommendation: Approve ✅

Great work on maintaining consistency across all the different SDKs and implementations!

@claude
Copy link

claude bot commented Nov 20, 2025

PR Review: Rename retry -> hibernate in runner protocol

Overview

This PR renames the retry field/parameter to hibernate across the runner protocol implementation. The renaming appears to better reflect the semantic meaning of the field - it's not about retrying an operation, but rather about hibernating the WebSocket connection.

Code Quality ✅

The changes are well-structured and follow the repository's conventions:

  • Consistent renaming across Rust and TypeScript codebases
  • Proper handling of version compatibility in the protocol versioning layer (v2 ↔ v3 conversions)
  • Clear separation of concerns maintained

Issues Found 🔴

1. Incomplete Renaming in stringify.ts (Critical)

Location: engine/sdks/typescript/runner/src/stringify.ts:61,64

The stringify function still references retry instead of hibernate:

const { code, reason, retry } = kind.val;
return `ToServerWebSocketClose{code: ${codeStr}, reason: ${reasonStr}, retry: ${retry}}`;

Should be:

const { code, reason, hibernate } = kind.val;
return `ToServerWebSocketClose{code: ${codeStr}, reason: ${reasonStr}, hibernate: ${hibernate}}`;

2. Type Signature Mismatch in websocket-tunnel-adapter.ts (Critical)

Location: engine/sdks/typescript/runner/src/websocket-tunnel-adapter.ts:20,35

The type signatures for the close callback still use retry instead of hibernate:

#closeCallback: (code?: number, reason?: string, retry?: boolean) => void;

closeCallback: (
    code?: number,
    reason?: string,
    retry?: boolean,
) => void,

Should be:

#closeCallback: (code?: number, reason?: string, hibernate?: boolean) => void;

closeCallback: (
    code?: number,
    reason?: string,
    hibernate?: boolean,
) => void,

3. Outdated Comments (Low Priority)

Location: engine/sdks/typescript/runner/src/tunnel.ts:80

Comment still references "retry":

// The WebSocket close event with retry is automatically sent when the

Should be:

// The WebSocket close event with hibernate is automatically sent when the

4. Protocol v2 Inconsistency (Informational)

Location: engine/sdks/schemas/runner-protocol/v2.bare:267

The v2 protocol schema still uses retry: bool. This is likely intentional for backward compatibility, and the conversion logic in versioned.rs handles the mapping correctly. However, it's worth noting for documentation purposes.

Potential Bugs 🐛

The incomplete renaming in issues #1 and #2 will cause:

  • TypeScript compilation errors or runtime errors when accessing kind.val.retry on a structure that now has kind.val.hibernate
  • Type safety issues with the callback signatures not matching actual usage

Performance Considerations ✅

No performance impact - this is purely a renaming refactor.

Security Concerns ✅

No security implications identified.

Test Coverage ⚠️

Recommendation: After fixing the issues above, ensure that:

  1. All TypeScript compilation passes without errors
  2. WebSocket hibernation behavior is tested end-to-end
  3. Protocol version conversion (v2 ↔ v3) is validated with the renamed field

Action Items

  1. ✅ Fix stringify.ts to use hibernate instead of retry
  2. ✅ Update type signatures in websocket-tunnel-adapter.ts
  3. ✅ Update comment in tunnel.ts:80
  4. ✅ Run TypeScript compilation to catch any remaining references
  5. ✅ Test WebSocket hibernation functionality

Summary

Good refactoring effort to improve semantic clarity! The core logic changes are correct, but the PR is incomplete - several files still reference the old retry naming. Please address the critical issues in stringify.ts and websocket-tunnel-adapter.ts before merging.

@claude
Copy link

claude bot commented Nov 20, 2025

Code Review

This PR renames the retry field to hibernate in the WebSocket close message across the runner protocol.

What's Good

  • Properly updated the BARE schema (v3.bare)
  • Updated all TypeScript and Rust implementations
  • Maintained backward compatibility in protocol versioning
  • Updated all usage sites in pegboard-gateway and pegboard-runner

Critical Issue Found

Missing Update in TypeScript Stringify File

The file engine/sdks/typescript/runner/src/stringify.ts at lines 61 and 64 still references retry instead of hibernate. This will cause TypeScript compilation errors.

Line 61 needs: const { code, reason, hibernate } = kind.val;
Line 64 needs the output string to reference hibernate instead of retry

Other Issues

Comment Position: In pegboard-gateway/src/lib.rs:404-408, the "Successful closure" comment was moved. Consider clarifying which branch is the successful closure.

Formatting Changes: The PR includes unrelated whitespace changes in tunnel.ts (lines 404-405, 611). Consider removing these.

Test Naming: The test test_websocket_retry in websocket.rs tests connection retry, not the hibernate field. Consider renaming to avoid confusion.

Security and Performance

  • No security concerns
  • No performance impact
  • Backward compatibility maintained

Status

Changes Requested - stringify.ts must be updated before merging.

@NathanFlurry NathanFlurry force-pushed the 11-13-chore_rename_retry_-_hibernate_in_runner_protocol branch from 274d094 to d44f1d1 Compare November 20, 2025 23:32
@NathanFlurry NathanFlurry force-pushed the 11-13-fix_udb_update_backoff_algo branch from 32b420c to 666b02e Compare November 20, 2025 23:32
@claude
Copy link

claude bot commented Nov 20, 2025

Code Review

Summary

This PR renames the 'retry' field to 'hibernate' in the runner protocol's ToServerWebSocketClose message. The change is a straightforward semantic rename that better reflects the intent of the field - indicating whether the WebSocket connection should hibernate rather than simply retry.

Positive Aspects

  1. Clear semantic improvement: The name 'hibernate' more accurately describes the behavior (putting the connection into a hibernation state) versus the generic term 'retry'.

  2. Comprehensive renaming: The change is applied consistently across all layers:

    • Protocol schema (v3.bare)
    • Rust implementations (pegboard-gateway, pegboard-runner)
    • TypeScript implementations (runner-protocol, runner, websocket-tunnel-adapter)
    • Version conversion functions (v2 ↔ v3 compatibility)
  3. Backward compatibility maintained: The versioning conversion functions properly map between close.retry (v2) and close.hibernate (v3) in both directions at versioned.rs:1288 and versioned.rs:1337.

  4. Correct method naming: The method __closeWithRetry was appropriately renamed to __closeWithHibernate in websocket-tunnel-adapter.ts:207.

Issues Found

1. Unintentional Formatting Changes (Minor)

The PR includes unintentional whitespace changes in TypeScript files that should be excluded:

tunnel.ts:404-405 - Changed from () => {} to () => { } (added space before closing brace)
tunnel.ts:610-611 - Changed indentation alignment

According to CLAUDE.md, the project prefers not to mix formatting changes with semantic changes. These should be reverted to keep the diff focused on the actual rename.

2. Comment Placement Changed (Minor)

In pegboard-gateway/src/lib.rs:407, the comment // Successful closure was moved from line 405 (before the if statement) to line 407 (in the else block). While this might be more accurate placement, it's an unrelated change that should either be mentioned in the commit message or be kept in the original position to minimize diff noise.

Recommendations

  1. Revert unintentional formatting changes in tunnel.ts (lines 404-405, 610-611) to keep the PR focused.

  2. Consider adding tests: No test coverage was found for the WebSocket hibernation behavior. Consider adding integration tests that verify:

    • WebSocket closes with hibernate: true trigger hibernation when can_hibernate is enabled
    • WebSocket closes with hibernate: false result in normal closure
    • Version conversion properly maps retry ↔ hibernate between protocol versions
  3. Documentation: If hibernation behavior is user-facing or important for operators, consider documenting it (though this may be out of scope for a simple rename).

Security Considerations

No security concerns identified. The change is a pure rename with no behavioral modifications.

Performance Considerations

No performance impact. The field size and serialization remain identical.

Final Verdict

This is a solid refactoring PR that improves code clarity. The main issues are minor formatting changes that should be cleaned up. Once the unintentional formatting changes are reverted, this is ready to merge.

Suggested Action: Request changes to revert formatting-only modifications, then approve.

@claude
Copy link

claude bot commented Nov 20, 2025

Code Review: Rename retry -> hibernate in runner protocol

Thank you for this PR! The rename from retry to hibernate is a good change for clarity. However, I found several locations where the old retry naming was not updated:

Issues Found

  1. Missing rename in stringify.ts (engine/sdks/typescript/runner/src/stringify.ts:61-64) - The debug/stringify function still references the old field name in both destructuring and string interpolation.

  2. Missing rename in WebSocketTunnelAdapter callback type (engine/sdks/typescript/runner/src/websocket-tunnel-adapter.ts:20) - The callback function signature parameter still uses retry instead of hibernate.

  3. Missing rename in v1->v2 version conversion (engine/sdks/rust/runner-protocol/src/versioned.rs:839) - The conversion still uses retry: false. This might be intentional if v2 still uses retry, but worth verifying.

Code Quality

Good aspects:

  • The v2 to v3 version conversion correctly maps retry to hibernate
  • All v3 schema files properly use the new hibernate naming
  • Comments were updated appropriately in pegboard-gateway
  • Whitespace formatting changes improve consistency

Minor formatting inconsistency in tunnel.ts:609-611 where indentation style changed from tabs to spaces.

Recommendation

Please update the three locations mentioned above to ensure consistency across the codebase. After fixing these issues, the PR will be ready to merge.


Review generated by Claude Code

@claude
Copy link

claude bot commented Nov 21, 2025

Code Review

This PR renames the retry field to hibernate in the ToServerWebSocketClose message of the runner protocol. This is a good semantic improvement that better reflects the actual behavior.

Strengths

  1. Better Naming Convention: The rename from retry to hibernate is much clearer and more accurately describes what the field does. Based on the documentation in docs/engine/HIBERNATING_WS.md, this field signals that the WebSocket should be hibernated (kept alive at the Guard layer while the runner sleeps), not retried.

  2. Complete Implementation: The changes are thorough and cover all necessary layers: Protocol schema (v3.bare), Generated TypeScript bindings, Generated Rust bindings, Version conversion logic (v2 to v3), Pegboard gateway usage, Pegboard runner usage, TypeScript runner SDK

  3. Backward Compatibility Handled: The version conversion functions in engine/sdks/rust/runner-protocol/src/versioned.rs:1285-1337 correctly map the old retry field to the new hibernate field for v2 to v3 conversions.

  4. Consistent Usage: The rename is applied consistently across the codebase, including internal method names like __closeWithRetry to __closeWithHibernate.

Issues Found

CRITICAL - Missed Update in stringify.ts

The stringify function still references the old field name at engine/sdks/typescript/runner/src/stringify.ts:61-64. The destructuring and string formatting still use retry instead of hibernate. This will cause incorrect logging and debugging output.

Minor - Comment Update

The comment in engine/sdks/typescript/runner/src/tunnel.ts:80 still says retry and should be updated to hibernate for consistency.

Other Observations

  1. Formatting Changes: The diff includes some unrelated formatting changes (whitespace in arrow functions at tunnel.ts:404-405,610). While harmless, these could have been avoided per the project guidelines about not running formatters.

  2. No Breaking Changes for Users: Since this is a protocol field rename with version conversion logic in place, existing v2 protocol users will not be affected.

  3. Documentation Already Correct: The documentation in docs/engine/HIBERNATING_WS.md:12 already uses the correct terminology, which confirms this rename aligns the code with the docs.

Recommendations

  1. Fix the stringify.ts bug - Update both the destructuring and the return string
  2. Update the comment in tunnel.ts:80
  3. Consider running tests to ensure the stringify function is covered and would catch this in the future

Summary

This is a good refactoring that improves code clarity. The implementation is mostly complete and handles backward compatibility correctly. However, there is one critical bug in the stringify function that needs to be fixed before merging.

@claude
Copy link

claude bot commented Nov 21, 2025

Code Review

Summary

This PR renames the retry field to hibernate in the runner protocol's WebSocket close message. The renaming is semantically clearer since the field indicates whether the connection should be hibernated (kept alive for potential resumption) rather than simply retried.

✅ Strengths

  1. Consistent Renaming: The change is applied consistently across all layers:

    • Schema definition (v3.bare)
    • Rust protocol implementation
    • TypeScript protocol implementation
    • Rust gateway/runner services
    • TypeScript runner SDK
  2. Backward Compatibility Handled: The version conversion functions in versioned.rs correctly map between v2 (retry) and v3 (hibernate) field names:

    • v2.retryv3.hibernate (line 1288)
    • v3.hibernatev2.retry (line 1337)
  3. Semantic Clarity: The new name hibernate better describes the behavior - putting the connection into a hibernated state rather than just retrying.

  4. Clean Refactoring: Method name __closeWithRetry__closeWithHibernate improves consistency.

🔍 Observations

  1. Unrelated Formatting Changes: Lines 404-405 and 611 in tunnel.ts contain formatting changes (spacing in empty functions and indentation) that are unrelated to the rename:

    resolve: () => { },
    reject: () => { },

    And line 611 has indentation changes. These should ideally be in a separate commit or avoided per the project's "Do not format the code yourself" guideline in CLAUDE.md.

  2. Logic Reordering: In pegboard-gateway/src/lib.rs (lines 404-408), the comment // Successful closure was moved from before the hibernate check to the else branch. This is actually an improvement since successful closure only happens when NOT hibernating.

🎯 Recommendations

Minor (Optional):

  • Consider reverting the unrelated formatting changes in tunnel.ts to keep the PR focused purely on the rename.

✅ Final Assessment

Approved - This is a clean, well-executed refactoring that improves code clarity. The rename is applied consistently across the entire codebase with proper backward compatibility handling. The only minor issue is some incidental formatting changes, but these don't affect functionality.

Testing Recommendations:

  • Verify WebSocket hibernation works correctly with v2 and v3 protocol clients
  • Test that non-hibernating closes still work as expected
  • Confirm protocol version conversion between v2/v3 maintains correct behavior

@claude
Copy link

claude bot commented Nov 21, 2025

PR Review: Rename retry -> hibernate in runner protocol

Summary

This PR renames the retry field to hibernate in the WebSocket close protocol across multiple SDKs (Rust and TypeScript). The change is a semantic improvement that better describes the behavior: when a WebSocket closes with this flag, the connection can be hibernated and potentially reconnected, rather than "retried" which implies the same operation is attempted again.

✅ Positive Aspects

  1. Consistent Naming: The rename is applied consistently across the protocol schema, generated code, and implementation files
  2. Clear Semantic Meaning: "hibernate" better conveys the intended behavior of allowing the connection to be suspended rather than truly retried
  3. Protocol Versioning: The change correctly maintains backward compatibility through the versioned protocol conversion in versioned.rs:1285-1337
  4. Minimal Surface Area: The change is focused and does not introduce unnecessary modifications

⚠️ Issues Found

CRITICAL - Missing Update in stringify.ts

The file engine/sdks/typescript/runner/src/stringify.ts still references retry instead of hibernate:

// Lines 61-64
const { code, reason, retry } = kind.val;
const codeStr = code === null ? "null" : code.toString();
const reasonStr = reason === null ? "null" : `"${reason}"`;
return `ToServerWebSocketClose{code: ${codeStr}, reason: ${reasonStr}, retry: ${retry}}`;

This needs to be updated to:

const { code, reason, hibernate } = kind.val;
// ...
return `ToServerWebSocketClose{code: ${codeStr}, reason: ${reasonStr}, hibernate: ${hibernate}}`;

📝 Minor Observations

  1. Whitespace Changes: The PR includes some unintentional whitespace changes in tunnel.ts:404-405 and tunnel.ts:611. While harmless, these could have been avoided:

    resolve: () => { },  // Added space before }
    reject: () => { },
  2. Documentation: Consider whether engine/contrib-docs/PEGBOARD_TUNNEL_RETRIES.md should be updated or renamed to reflect the new terminology. The doc heavily uses "retry" terminology but the actual protocol field is now "hibernate". This could cause confusion for future developers.

  3. Comment Accuracy: In pegboard-gateway/src/lib.rs:407, the comment "Successful closure" remains unchanged, which is accurate and appropriate.

🧪 Testing Recommendations

  1. Verify stringify output: Ensure that debug/logging output correctly shows "hibernate" instead of "retry"
  2. Protocol compatibility: Test that old clients using v2 protocol can still communicate with v3 protocol servers (the versioned conversion should handle this)
  3. WebSocket lifecycle: Verify that WebSocket hibernation still functions correctly after the rename

📊 Code Quality: 7/10

Scoring Breakdown:

  • Consistency: 8/10 (missing stringify.ts)
  • Maintainability: 8/10 (clean rename)
  • Testing: N/A (no tests modified, but existing tests should cover this)
  • Documentation: 6/10 (potential confusion with PEGBOARD_TUNNEL_RETRIES.md)

✅ Action Items

Must Fix:

  • Update engine/sdks/typescript/runner/src/stringify.ts to use hibernate instead of retry

Should Consider:

  • Update or add note to engine/contrib-docs/PEGBOARD_TUNNEL_RETRIES.md explaining the field is now called hibernate
  • Run tests to verify stringify output is correct
  • Consider reverting unintentional whitespace changes in tunnel.ts

Overall Assessment

This is a good semantic improvement to the codebase. The core rename is done correctly, but there is one critical missing update in the stringify functionality that would cause incorrect logging/debugging output. Once that is fixed, this PR will be ready to merge.


Review generated with assistance from Claude Code

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 21, 2025

Merge activity

  • Nov 21, 2:40 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 21, 2:42 AM UTC: CI is running for this pull request on a draft pull request (#3515) due to your merge queue CI optimization settings.
  • Nov 21, 2:43 AM UTC: Merged by the Graphite merge queue via draft PR: #3515.

@graphite-app graphite-app bot closed this Nov 21, 2025
@graphite-app graphite-app bot deleted the 11-13-chore_rename_retry_-_hibernate_in_runner_protocol branch November 21, 2025 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants